[X86] Use Register in X86InstrBuilder.h. NFC #130514
Merged
+17
−24
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
I had to give the X86AddressMode Base union a name and a constructor so it would default to Register. This means the Base.Reg = 0 in the X86AddressMode constructor is no longer needed.
Member
|
@llvm/pr-subscribers-backend-x86 Author: Craig Topper (topperc) ChangesI had to give the X86AddressMode Base union a name and a constructor so it would default to Register. This means the Base.Reg = 0 in the X86AddressMode constructor is no longer needed. I changed all the places that used addReg(0) to addReg(Register()). Full diff: https://github.com/llvm/llvm-project/pull/130514.diff 1 Files Affected:
diff --git a/llvm/lib/Target/X86/X86InstrBuilder.h b/llvm/lib/Target/X86/X86InstrBuilder.h
index 45c5f8aa82e97..e1b056f225b6c 100644
--- a/llvm/lib/Target/X86/X86InstrBuilder.h
+++ b/llvm/lib/Target/X86/X86InstrBuilder.h
@@ -40,27 +40,20 @@ namespace llvm {
/// with BP or SP and Disp being offsetted accordingly. The displacement may
/// also include the offset of a global value.
struct X86AddressMode {
- enum {
- RegBase,
- FrameIndexBase
- } BaseType;
+ enum { RegBase, FrameIndexBase } BaseType = RegBase;
- union {
- unsigned Reg;
+ union BaseUnion {
+ Register Reg;
int FrameIndex;
- } Base;
- unsigned Scale;
- unsigned IndexReg;
- int Disp;
- const GlobalValue *GV;
- unsigned GVOpFlags;
+ BaseUnion() : Reg() {}
+ } Base;
- X86AddressMode()
- : BaseType(RegBase), Scale(1), IndexReg(0), Disp(0), GV(nullptr),
- GVOpFlags(0) {
- Base.Reg = 0;
- }
+ unsigned Scale = 1;
+ Register IndexReg;
+ int Disp = 0;
+ const GlobalValue *GV = nullptr;
+ unsigned GVOpFlags = 0;
void getFullAddress(SmallVectorImpl<MachineOperand> &MO) {
assert(Scale == 1 || Scale == 2 || Scale == 4 || Scale == 8);
@@ -121,32 +114,36 @@ static inline X86AddressMode getAddressFromInstr(const MachineInstr *MI,
/// with no scale, index or displacement. An example is: DWORD PTR [EAX].
///
static inline const MachineInstrBuilder &
-addDirectMem(const MachineInstrBuilder &MIB, unsigned Reg) {
+addDirectMem(const MachineInstrBuilder &MIB, Register Reg) {
// Because memory references are always represented with five
// values, this adds: Reg, 1, NoReg, 0, NoReg to the instruction.
- return MIB.addReg(Reg).addImm(1).addReg(0).addImm(0).addReg(0);
+ return MIB.addReg(Reg)
+ .addImm(1)
+ .addReg(Register())
+ .addImm(0)
+ .addReg(Register());
}
/// Replace the address used in the instruction with the direct memory
/// reference.
static inline void setDirectAddressInInstr(MachineInstr *MI, unsigned Operand,
- unsigned Reg) {
+ Register Reg) {
// Direct memory address is in a form of: Reg/FI, 1 (Scale), NoReg, 0, NoReg.
MI->getOperand(Operand).ChangeToRegister(Reg, /*isDef=*/false);
MI->getOperand(Operand + 1).setImm(1);
- MI->getOperand(Operand + 2).setReg(0);
+ MI->getOperand(Operand + 2).setReg(Register());
MI->getOperand(Operand + 3).ChangeToImmediate(0);
- MI->getOperand(Operand + 4).setReg(0);
+ MI->getOperand(Operand + 4).setReg(Register());
}
static inline const MachineInstrBuilder &
addOffset(const MachineInstrBuilder &MIB, int Offset) {
- return MIB.addImm(1).addReg(0).addImm(Offset).addReg(0);
+ return MIB.addImm(1).addReg(Register()).addImm(Offset).addReg(Register());
}
static inline const MachineInstrBuilder &
addOffset(const MachineInstrBuilder &MIB, const MachineOperand& Offset) {
- return MIB.addImm(1).addReg(0).add(Offset).addReg(0);
+ return MIB.addImm(1).addReg(Register()).add(Offset).addReg(Register());
}
/// addRegOffset - This function is used to add a memory reference of the form
@@ -154,21 +151,21 @@ addOffset(const MachineInstrBuilder &MIB, const MachineOperand& Offset) {
/// displacement. An example is: DWORD PTR [EAX + 4].
///
static inline const MachineInstrBuilder &
-addRegOffset(const MachineInstrBuilder &MIB,
- unsigned Reg, bool isKill, int Offset) {
+addRegOffset(const MachineInstrBuilder &MIB, Register Reg, bool isKill,
+ int Offset) {
return addOffset(MIB.addReg(Reg, getKillRegState(isKill)), Offset);
}
/// addRegReg - This function is used to add a memory reference of the form:
/// [Reg + Reg].
static inline const MachineInstrBuilder &
-addRegReg(const MachineInstrBuilder &MIB, unsigned Reg1, bool isKill1,
- unsigned SubReg1, unsigned Reg2, bool isKill2, unsigned SubReg2) {
+addRegReg(const MachineInstrBuilder &MIB, Register Reg1, bool isKill1,
+ unsigned SubReg1, Register Reg2, bool isKill2, unsigned SubReg2) {
return MIB.addReg(Reg1, getKillRegState(isKill1), SubReg1)
.addImm(1)
.addReg(Reg2, getKillRegState(isKill2), SubReg2)
.addImm(0)
- .addReg(0);
+ .addReg(Register());
}
static inline const MachineInstrBuilder &
@@ -189,7 +186,7 @@ addFullAddress(const MachineInstrBuilder &MIB,
else
MIB.addImm(AM.Disp);
- return MIB.addReg(0);
+ return MIB.addReg(Register());
}
/// addFrameReference - This function is used to add a reference to the base of
@@ -224,10 +221,13 @@ addFrameReference(const MachineInstrBuilder &MIB, int FI, int Offset = 0) {
///
static inline const MachineInstrBuilder &
addConstantPoolReference(const MachineInstrBuilder &MIB, unsigned CPI,
- unsigned GlobalBaseReg, unsigned char OpFlags) {
+ Register GlobalBaseReg, unsigned char OpFlags) {
//FIXME: factor this
- return MIB.addReg(GlobalBaseReg).addImm(1).addReg(0)
- .addConstantPoolIndex(CPI, 0, OpFlags).addReg(0);
+ return MIB.addReg(GlobalBaseReg)
+ .addImm(1)
+ .addReg(Register())
+ .addConstantPoolIndex(CPI, 0, OpFlags)
+ .addReg(Register());
}
} // end namespace llvm
|
phoebewang
reviewed
Mar 10, 2025
| MI->getOperand(Operand).ChangeToRegister(Reg, /*isDef=*/false); | ||
| MI->getOperand(Operand + 1).setImm(1); | ||
| MI->getOperand(Operand + 2).setReg(0); | ||
| MI->getOperand(Operand + 2).setReg(Register()); |
Contributor
There was a problem hiding this comment.
I think it makes readability a little worse. Can we keep it 0 or X86::NoRegister?
Collaborator
Author
There was a problem hiding this comment.
I'm not sure why you say it makes readability worse. Can you say more? I think 0 is easily confused with immediate.
I'm not sure if we'll keep the implicit conversion from unsigned to Register long term.
Contributor
There was a problem hiding this comment.
That's only a personal feeling. We have used 0 as no register for a long time. setReg(Register()) doesn't look intuitive to me.
In the future, if setReg only accepts Register, I'd like it has a default value, so that we can use setReg() for the purpose. Or define constexpr Register NoRegister and use NoRegister everywhere?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I had to give the X86AddressMode Base union a name and a constructor so it would default to Register. This means the Base.Reg = 0 in the X86AddressMode constructor is no longer needed.